Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Setup SSO for Admin app #3138

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Setup SSO for Admin app #3138

wants to merge 8 commits into from

Conversation

phil-l-brockwell
Copy link
Contributor

📝 A short description of the changes

  • Replace existing authentication for Admin app with OmniAuth and integrate with the DBT Staff SSO

🔗 Link to the relevant story (or stories)

:shipit: Deployment implications

  • Integrate with the DBT staff SSO once we have some test credentials before merging (tested in development enviroment with an instance of: doorkeeper.
  • Can we drop the redundant authorisation db fields now? eg: encrypted_password/authy_id/authy_enabled/last_sign_in_with_authy/etc

✅ Checklist

  • Features that cannot go live are behind a feature flag/env var or specify deploy date and open PR as draft
  • I have checked that commit messages make sense and explain the reasoning for each change
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have squashed any unnecessary or part-finished commits

🖼️ Screenshots (if appropriate - no PII/Prod data):

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 4, 2024
View reviewed changes
app/controllers/admins/omniauth_callbacks_controller.rb
@@ -0,0 +1,33 @@
class Admins::OmniauthCallbacksController < Devise::OmniauthCallbacksController
skip_before_action :verify_authenticity_token, only: :developer

Check failure

Code scanning / CodeQL

CSRF protection weakened or disabled High

Potential CSRF vulnerability due to forgery protection being disabled or weakened.

Copilot Autofix AI about 1 month ago

To fix the CSRF vulnerability, we should re-enable CSRF protection for the developer action. This can be done by removing the skip_before_action :verify_authenticity_token, only: :developer line. Additionally, we can add a safeguard to ensure that the developer action is only accessible in development environments by using a more secure approach, such as an environment-specific route constraint.

Suggested changeset 1
app/controllers/admins/omniauth_callbacks_controller.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/controllers/admins/omniauth_callbacks_controller.rb b/app/controllers/admins/omniauth_callbacks_controller.rb
--- a/app/controllers/admins/omniauth_callbacks_controller.rb
+++ b/app/controllers/admins/omniauth_callbacks_controller.rb
@@ -1,3 +1,2 @@
 class Admins::OmniauthCallbacksController < Devise::OmniauthCallbacksController
-  skip_before_action :verify_authenticity_token, only: :developer
 
EOF
@@ -1,3 +1,2 @@
class Admins::OmniauthCallbacksController < Devise::OmniauthCallbacksController
skip_before_action :verify_authenticity_token, only: :developer

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@phil-l-brockwell